At Mon, 8 Aug 2022 11:53:19 -0400, Greg Stark <stark@mit.edu> wrote in > I'm trying to wrap my head around the shared memory stats collector > infrastructure from > <20220406030008.2qxipjxo776dwnqs@alap3.anarazel.de> committed in > 5891c7a8ed8f2d3d577e7eea34dacff12d7b6bbd. > > I have one question about locking -- afaics there's nothing protecting > reading the shared memory stats. There is an lwlock protecting > concurrent updates of the shared memory stats, but that lock isn't > taken when we read the stats. Are we ok relying on atomic 64-bit reads > or is there something else going on that I'm missing? > > In particular I'm looking at pgstat.c:847 in pgstat_fetch_entry() > which does this: > > memcpy(stats_data, > pgstat_get_entry_data(kind, entry_ref->shared_stats), > kind_info->shared_data_len); > > stats_data is the returned copy of the stats entry with all the > statistics in it. But it's copied from the shared memory location > directly using memcpy and there's no locking or change counter or > anything protecting this memcpy that I can see. We take LW_SHARED while creating a snapshot of fixed-numbered stats. On the other hand we don't for variable-numbered stats. I agree to you, that we need that also for variable-numbered stats. If I'm not missing something, it's strange that pgstat_lock_entry() only takes LW_EXCLUSIVE. The atached changes the interface of pgstat_lock_entry() but there's only one user since another read of shared stats entry is not using reference. Thus the interface change might be too much. If I just add bare LWLockAcquire/Release() to pgstat_fetch_entry,the amount of the patch will be reduced. regards. -- Kyotaro Horiguchi NTT Open Source Software Center